Fix/disable RedHat System.Globalization, System.IO.Pipes, System.IO.FileSystem failures #24340

krwq · 2017-09-29T18:03:26Z

Fixes: https://github.com/dotnet/corefx/issues/24304
Fixes: https://github.com/dotnet/corefx/issues/23628
Fixes: https://github.com/dotnet/corefx/issues/23630 (port disable from master)
Contributes to: https://github.com/dotnet/corefx/issues/21920

Original fix made by @tarekgh will require changes to CoreCLR (add ICU version detection to native code) which is likely not appropriate as servicing change which does not fix any product bugs.

This PR replaces #24338 which was against dev/release/2.0.0

mmitche · 2017-09-29T19:26:22Z

@dotnet-bot test this please

krwq · 2017-09-29T20:57:31Z

@danmosemsft Is it ok to merge @janvorli's PRs here to enable RHEL 6? I'd like to test this before merging this PR:
https://github.com/dotnet/corefx/pull/24285/commits
https://github.com/dotnet/corefx/pull/24331/commits

tarekgh · 2017-09-29T21:51:38Z

@krwq I think you are missing some part in how we detect RedHat mainly in the method ParseOsReleaseFile. I'll try to send you a link to the code we have done in 2.1 to do that

tarekgh · 2017-09-29T21:53:41Z

@krwq please look at the implementation of the method ParseOsReleaseFile which we need to port.

https://github.com/dotnet/corefx/blob/master/src/CoreFx.Private.TestUtilities/src/System/PlatformDetection.Unix.cs#L84

krwq · 2017-09-29T22:10:38Z

@tarekgh it's already there but in PlatformDetection.cs and not PlatformDetection.Unix.cs

EDIT: aahh, I see - there is some special stuff about RHEL which is missing - thank you!

janvorli · 2017-09-29T23:54:12Z

src/Common/tests/System/PlatformDetection.cs

@@ -161,6 +161,7 @@ public static bool IsWinRT
        public static bool IsNotFedoraOrRedHatOrCentos => !IsDistroAndVersion("fedora") && !IsDistroAndVersion("rhel") && !IsDistroAndVersion("centos");

        public static bool IsFedora => IsDistroAndVersion("fedora");
+        public static bool IsRedHat69 => IsDistroAndVersion("rhel", "6.9") || IsDistroAndVersion("rhl", "6.9");


I would check just the major version here, the minor one should not change the behavior, I believe.

I would leave this as is for consistency with master - in this particular case we need to tell which version of ICU shipped with RedHat, I'm not sure what are the differences between 6.9 and other 6.X versions.

There is no usable ICU version that ships with any version RedHat 6. We tell the users to install version 57.1 binary from the tarball provided on the ICU site. But even if there was one, RedHat never changes the major versions of libraries they ship when they bump the minor version. So 6.0, 6.1, ... 6.9 all would have the same major version of the library.

@janvorli what @krwq meant is in 2.1 branch we detect the ICU version at runtime. so we don't care how the ICU get installed on the machine anyway. The right way to check the globalization behavior is to check the ICU version and not the OS version. We don't do that in v 2.0 servicing branches because this will need a change in the release bits (System.Globalzation.Native) and not the test bits only. This is why we are just doing the minimum needed in the servicing branch to have the tests not failing there.

danmoseley

You need to ping shiproom for ok to submit.

danmoseley · 2017-10-05T19:56:00Z

@krwq they are about to lock down the branch, can you please get shiproom OK?

@dotnet-bot test Innerloop Tizen armel Debug Cross Build Innerloop Tizen armel Release Cross Build

danmoseley · 2017-10-05T19:56:16Z

@dotnet-bot test Innerloop Tizen armel Release Cross Build

krwq · 2017-10-05T21:27:33Z

@danmosemsft I am currently not able to test if RedHat is fixed without pushing my commit - there is no CI or any complete instructions how to set up RedHat 6.9 (closest are @janvorli's instructions https://github.com/dotnet/core/blob/master/Documentation/build-and-install-rhel6-prerequisites.md but they only let you run app and not build corefx) - I'm working on that right now. Could this PR wait a little? It doesn't seem like we will be changing any product code here..

danmoseley · 2017-10-05T22:02:45Z

@krwq sure. If it's too much of a pain we could ask to merge anyway, the change seems really hard to get wrong. Did you ask MattGal whether you could borrow a build machine that's already set up?

janvorli · 2017-10-05T22:03:00Z

@krwq you can test it in docker container, the image microsoft/dotnet-buildtools-prereqs:centos-6-783abde-20171304101322 that we use in the lab for building has all the necessary prerequisites.

On a local Linux machine or VM, make sure you have docker installed and then do this:

sudo docker run --net host -it --name my_centos_6 microsoft/dotnet-buildtools-prereqs:centos-6-783abde-20171304101322 /usr/bin/scl enable python27 bash

That will get you inside the docker container as a 'root' user. You can then clone the corefx repo using git and build / test everything.

The important thing though is to prefix invocation of all the build.sh, run-tests.sh etc with LD_LIBRARY_PATH=/usr/local/lib. So e.g.

LD_LIBRARY_PATH=/usr/local/lib ./build.sh

Once you are done with your work, exit the container using the “exit” command.

If you want to resume working on the CentOS 6 stuff, use the following command to re-enter the container:

sudo docker start -ai my_centos_6

krwq · 2017-10-06T18:23:03Z

@janvorli thank you! I was able to build release/2.0.0 successfuly (on master I still get build errors). I think there might be a problem with the RedHat docker image:

> cat /etc/redhat-release
CentOS release 6.9 (Final)

this is causing OS detection to not work correctly - I'll update the test to accept both CentOS and RedHat for now since there shouldn't be any difference

janvorli · 2017-10-06T18:31:04Z

@krwq rhel and centos are equivalent and so at all places of detection I was adding, both systems get rid "rhel.6" intentionally. I think we should follow the suite here as well so that we have it detected everywhere the same way.

krwq · 2017-10-06T18:59:48Z

@janvorli thanks! I'll unify rhel and centos in this PR

janvorli · 2017-10-06T20:17:58Z

src/Common/tests/System/PlatformDetection.cs


        public static bool IsFedora => IsDistroAndVersion("fedora");
+        public static bool IsRedHatFamily => IsRedHatFamilyAndVersion();
+        public static bool IsRedHatFamily7 => IsRedHatFamilyAndVersion("7");


For CentOS and RHEL 7, there is a difference in the version reported via /etc/os-release. CentOS reports just 7, but RHEL reports 7.x where x is the minor version. So this needs to be taken into account, unless I am missing something, there is no version normalization code anywhere.
That is different for version 6 where there is no /etc/os-release and the /etc/redhat-release contains version 6.x, that means including the minor version for both RHEL and CentOS.

ok, i've added additional checks in IsRedHatFamilyAndVersion and RedHatVersionEqual

tarekgh · 2017-10-07T00:43:57Z

@krwq thanks for cleaning up this area and doing the right thing. I have added a couple of comments.

tarekgh · 2017-10-06T23:51:26Z

src/Common/tests/System/PlatformDetection.cs

+                            {
+                                ret.Id = "centos";
+                            }
+                            else if (line.StartsWith("Red Hat", StringComparison.OrdinalIgnoreCase))


@krwq could you please remove this else block? it is not really needed and can confuse people for having some non-standard redhat distro. Also, could you please remove it from the master too when you are applying some changes there as we talked before?

tarekgh · 2017-10-07T00:28:34Z

src/Common/tests/System/PlatformDetection.cs

@@ -158,9 +158,13 @@ public static bool IsWinRT
        public static bool IsWindowsSubsystemForLinux => m_isWindowsSubsystemForLinux.Value;
        public static bool IsNotWindowsSubsystemForLinux => !IsWindowsSubsystemForLinux;

-        public static bool IsNotFedoraOrRedHatOrCentos => !IsDistroAndVersion("fedora") && !IsDistroAndVersion("rhel") && !IsDistroAndVersion("centos");
+        public static bool IsNotFedoraOrRedHatFamily => !IsDistroAndVersion("fedora") && !IsRedHatFamily;

        public static bool IsFedora => IsDistroAndVersion("fedora");


please move IsFedora and IsRedHatFamily before the line of IsNotFedoraOrRedHatFamily to ensure the right order of the static initialization.

tarekgh · 2017-10-07T00:29:20Z

src/Common/tests/System/PlatformDetection.cs

@@ -158,9 +158,13 @@ public static bool IsWinRT
        public static bool IsWindowsSubsystemForLinux => m_isWindowsSubsystemForLinux.Value;
        public static bool IsNotWindowsSubsystemForLinux => !IsWindowsSubsystemForLinux;

-        public static bool IsNotFedoraOrRedHatOrCentos => !IsDistroAndVersion("fedora") && !IsDistroAndVersion("rhel") && !IsDistroAndVersion("centos");
+        public static bool IsNotFedoraOrRedHatFamily => !IsDistroAndVersion("fedora") && !IsRedHatFamily;


Use IsFedora instead of IsDistroAndVersion("fedora"). ensure IsFedora is moved before this line

tarekgh · 2017-10-07T00:31:15Z

src/Common/tests/System/PlatformDetection.cs


        public static bool IsFedora => IsDistroAndVersion("fedora");
+        public static bool IsRedHatFamily => IsRedHatFamilyAndVersion();


please add comment that this is cover RedHat and Centos. Just in case if anyone want to check Centos and don't know it is covered by this property.

tarekgh · 2017-10-07T00:41:50Z

src/Common/tests/System/PlatformDetection.cs

+                IdVersionPair v = ParseOsReleaseFile();
+                bool isRedHat = v.Id == "rhel";
+                // RedHat includes minor version. We need to account for that when comparing
+                if ((isRedHat || v.Id == "centos") && (versionId == null || v.VersionId == versionId || (isRedHat && RedHatVersionEqual(versionId, v.VersionId))))


the version check logic can be written in a more global way which can include checking only concerned parts of the version. this can be done in 2 ways:

1- either you do versionId.Split('.') which will produce an array of strings of the version parts and also do v.VersionId.Split('.'). Then try to check versionId parts against v.VersionId parts (regardless how many parts is in versionId).
2- other idea is when we are reading the distro info, we read the version string and convert it to Version struct. then we can just use numbers instead of strings all the time.

if we do one of these ideas, we don't have to distinguish between isRedHat or not as you are doing in your code.

pending (string.split) but might get changed after fixing Eric's comment

eerhardt · 2017-10-07T19:11:23Z

src/Common/tests/System/PlatformDetection.cs

-                ret.VersionId = versionId.Substring(1, versionId.Length - 2);
-            }
+                string fileName = null;
+                if (File.Exists("/etc/redhat-release"))


Would it be beneficial to use the RuntimeEnvironment class here? It is already doing all this logic, and it is how our customers get the current RID of the machine, and what version of RedHat we are on, etc.

I know the dependency would be a little inverted here (corefx code referencing core-setup code). But we do that today when using the "shared framework" to run tests. So there is precedent.

/cc @weshaggard

That would make core-setup necessary for running dotnet apps. Now it is not needed - you can just build coreclr and corefx and use corerun / coreconsole for running your app. I believe that's what the Samsung folks are using.
If we want to unify the detection code (which would be great), I think we should rather move it to corefx and let core-setup use it.

I thought this was only for test code?

As for moving the real logic into CoreFX- yes please!

Ah, I am sorry, I've missed that it is for tests only. You are right.

@eerhardt does RuntimeEnvironment is a public class which can easily take a dependency on? Also, how easy we can add more properties to it? I am asking because for the tests we need to be flexible of adding more properties to the PaltformDetection code and customize it to fit the cases needed in the tests. in another word, for tests, I prefer the flexibility even if we duplicate some of such features.

Another thing, this PR is for 2.0 servicing branch which I prefer to minimize the changes here, so if we decide to take a dependency on RuntimeEnvironment, I prefer to do it in master and not in 2.0 branch.

@weshaggard why this functionality is not exposed from RuntimeInformation class instead? why have we introduced RuntimeEnvironment while this can fit better in RuntimeInformation?

It may get moved down eventually but today the RID stuff is only on the RuntimeEnviroment class.

While I see advantage of using RuntimeEnvironment here I'll exclude this change request from 2.0.0 PR as it will introduce too much churn because PlatformDetection currently lives in the Common folder meaning every test project will now have to depend on PlatformAbstraction. I'll change it in the PR against master as we can change just test utils project.

If you strongly feel we should do that in 2.0.0 branch I'll do it but currently think we should limit 2.0.0 changes to minimum.

To clarify, I'll do this for master branch but will exclude for servicing branch (because too many projects will have to be touched)

Another thing, this PR is for 2.0 servicing branch which I prefer to minimize the changes here, so if we decide to take a dependency on RuntimeEnvironment, I prefer to do it in master and not in 2.0 branch.

I'll do this for master branch but will exclude for servicing branch (because too many projects will have to be touched)

Works for me. Thanks.

krwq · 2017-10-09T19:53:38Z

@vancem I believe the OSX failure might be related to your PR: #24342

vancem · 2017-10-09T20:16:06Z

Please ignore the failure in BasicEventSourceTests.TestEventCounter.Test_Write_Metric_EventListener.
The tests is flaky, it has already been fixed in master. I will disable it here.

krwq · 2017-10-09T20:36:12Z

Thank you @vancem! Merging since the failures are unrelated

krwq requested a review from tarekgh September 29, 2017 18:03

dnfclas added the cla-already-signed label Sep 29, 2017

krwq mentioned this pull request Sep 29, 2017

[WIP] Fix RedHat System.Globalization failures #24338

Closed

janvorli reviewed Sep 29, 2017

View reviewed changes

karelz added the area-System.Globalization label Oct 2, 2017

karelz assigned krwq Oct 2, 2017

karelz added this to the 2.0.x milestone Oct 2, 2017

danmoseley approved these changes Oct 5, 2017

View reviewed changes

krwq added 2 commits October 6, 2017 10:47

fix System.Globalization RedHat failure

5d60299

add RedHat version detection

8f39758

change test behavior also on centos 6.9

a85637c

krwq force-pushed the rh_test_failures branch from 531e4ee to a85637c Compare October 6, 2017 18:34

unify RedHat and CentOS detection

f4c0b52

janvorli reviewed Oct 6, 2017

View reviewed changes

krwq added 3 commits October 6, 2017 13:32

disable failing System.IO.Pipes on RedHatFamily69

f7d387a

Disable Unix_NotFoundDirectory_ReadOnlyVolume

f6df665

fix version comparison when only major is provided on RedHat

6848bbf

krwq changed the title ~~[WIP] Fix RedHat System.Globalization failures~~ Fix RedHat System.Globalization, System.IO.Pipes, System.IO.FileSystem failures Oct 6, 2017

krwq changed the title ~~Fix RedHat System.Globalization, System.IO.Pipes, System.IO.FileSystem failures~~ Fix/disable RedHat System.Globalization, System.IO.Pipes, System.IO.FileSystem failures Oct 6, 2017

remove confusing checks for non-standard RedHat distro (rhl)

59088fc

tarekgh suggested changes Oct 7, 2017

View reviewed changes

eerhardt reviewed Oct 7, 2017

View reviewed changes

clean up version comparison and reuse properties

b7702f2

tarekgh approved these changes Oct 9, 2017

View reviewed changes

krwq merged commit 00788da into dotnet:release/2.0.0 Oct 9, 2017


		public static bool IsFedora => IsDistroAndVersion("fedora");
		public static bool IsRedHatFamily => IsRedHatFamilyAndVersion();

Fix/disable RedHat System.Globalization, System.IO.Pipes, System.IO.FileSystem failures #24340

Fix/disable RedHat System.Globalization, System.IO.Pipes, System.IO.FileSystem failures #24340

Conversation

krwq commented Sep 29, 2017 • edited Loading

mmitche commented Sep 29, 2017

krwq commented Sep 29, 2017 • edited Loading

tarekgh commented Sep 29, 2017

tarekgh commented Sep 29, 2017 • edited Loading

krwq commented Sep 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley left a comment

Choose a reason for hiding this comment

danmoseley commented Oct 5, 2017

danmoseley commented Oct 5, 2017

krwq commented Oct 5, 2017

danmoseley commented Oct 5, 2017

janvorli commented Oct 5, 2017

krwq commented Oct 6, 2017

janvorli commented Oct 6, 2017

krwq commented Oct 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarekgh commented Oct 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarekgh Oct 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarekgh Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krwq Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krwq commented Oct 9, 2017

vancem commented Oct 9, 2017

krwq commented Oct 9, 2017

krwq commented Sep 29, 2017 •

edited

Loading

krwq commented Sep 29, 2017 •

edited

Loading

tarekgh commented Sep 29, 2017 •

edited

Loading

krwq commented Sep 29, 2017 •

edited

Loading

tarekgh Oct 7, 2017 •

edited

Loading

tarekgh Oct 9, 2017 •

edited

Loading

krwq Oct 9, 2017 •

edited

Loading